Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: maintain separate cache for native modules #6165

Closed
wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

module

Description of change

require.cache is often used to "unload" required modules for testing purposes.
This don't quite works for native purposes, because you can't load them twice.
The solution is to maintain independent cache for native modules so we can
create new module instance without invoking dlopen.

Result of realpath is used as cache key to avoid loading same module twice
on case-insensitive file systems.

Fixes: #6160

@vkurchatkin
Copy link
Contributor Author

A couple of caveats that come to my mind:

  • Native module could be explicitly initialised from js, so this could fail if js module is reevaluated;
  • exports is attached to one module object, but module initialiser was called with a different module object, so some inconsistencies are possible.

Still it seems better than just failing.

@thefourtheye thefourtheye added the module Issues and PRs related to the module subsystem. label Apr 12, 2016
@@ -1422,6 +1441,7 @@ void InitFs(Local<Object> target,
env->SetMethod(target, "readdir", ReadDir);
env->SetMethod(target, "internalModuleReadFile", InternalModuleReadFile);
env->SetMethod(target, "internalModuleStat", InternalModuleStat);
env->SetMethod(target, "internalModuleRealpath", InternalModuleRealpath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: internalModuleRealPath

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 12, 2016
@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

Tentatively marking this as a semver-major due to the nature of change.

@bnoordhuis
Copy link
Member

Not a fan. A lot of complexity for something that can be addressed by a one-liner in the documentation.

@vkurchatkin
Copy link
Contributor Author

@bnoordhuis it also fixes some other cases of loading the same addon twice

@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

@vkurchatkin ... could those cases be separated out? That is, separate the fix that addresses the addons being loaded twice from the rest of it? For the cache clearing case, I'm wondering if providing a simple require.clearCache() API that removes everything except addons would be enough here? (not 100% sure that even makes sense tho ;-) ...)

@Fishrock123
Copy link
Contributor

@jasnell I'm pretty sure this is that fix.

@vkurchatkin
Copy link
Contributor Author

@jasnell @Fishrock123 yep, using real path as cache key also solves crash when requiring Foo and foo, and foo has native module inside. Probably there are other similar cases, such as using symlinks

@jasnell
Copy link
Member

jasnell commented Apr 13, 2016

@Fishrock123 ... sorry, I wasn't clear. I meant (a) separating out the commits and (b) rather than having the separate const nativeModuleCache = {} provide a method on require that removes only the non-native modules from the existing cache.

@vkurchatkin
Copy link
Contributor Author

provide a method on require that removes only the non-native modules from the existing cache

This won't really fix existing code, so it will work only for users that know about this problem and change their code to avoid it. This is already possible to implement in userland by filtering out paths with .node extension.

@jasnell
Copy link
Member

jasnell commented Apr 13, 2016

yep, good point. I generally agree with @bnoordhuis with regards to the additional complexity of this but I understand the reasoning behind it. I guess I'm +0 on it for now.

@Fishrock123
Copy link
Contributor

This doesn't really seem all that complex to me. I'm not particularly against it.

@bnoordhuis
Copy link
Member

The nature of the module system is such that if we land this change, we're stuck with it forever.

If that doesn't convince you: it's a workaround for bad user expectations. That's what documentation is for.

@vkurchatkin
Copy link
Contributor Author

bad user expectations

Users expect things to work, if that's what you mean. It's a good thing that we handle some cases that previously resulted in crash, isn't it?

The other option is to deprecate mutating cache altogether. Everything else is a half measure.

@bnoordhuis
Copy link
Member

You know what I mean. The OP in #6160 assumed that add-ons could be unloaded by deleting from the cache, but they can't because of technical reasons.

If your position is that we hide that detail with a shadow cache, then that's a valid position to take - but it's one I don't agree with. :-)

@rvagg
Copy link
Member

rvagg commented Apr 14, 2016

I'm +1 on doing something about this but +0 on this change and unfortunately don't have a better idea just now. While I have sympathy for @bnoordhuis' position I don't agree with "The nature of the module system is such that if we land this change, we're stuck with it forever" on this change, there's no reason we couldn't undo it in another semver-major. It also seems that a lot of the complexity of this change comes from introducing internalModuleRealPath which seems to be addressing a completely separate issue, so are there two issues rolled up in this PR? Should we have separate test cases for those? Perhaps you'd make more headway here if you separate them into two PRs @vkurchatkin?

One concern with this solution is we'd be hiding the cache from users when there are good reasons to expose that data for native modules just like standard modules. An ideal solution here would be to prevent them from deleting cached data for native modules. Perhaps one way of doing this that wouldn't involve a huge perf hit would be to use an internal cache for native modules and each time it's queried, copy the key/value to require.cache, so we maintain an accurate view via require.cache but don't let any edits of the data there to impact on native module loading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants